clinic sites #159
clinic sites #159
Conversation
|
@gniezen any guidance on how to check up on these failures? I don't seem to be able to log in to redocly. Have I missed some instructions somewhere? |
ewollesen
force-pushed
the
eric-sites
branch
2 times, most recently
from
ac17368 to
9e787bc
Compare
|
|
@ewollesen We only have three seats for Redocly at the moment, but it seems they automatically add all repo participants when there's a new PR, so the error was that we're exceeding the seat limit. Let's ignore Redocly errors for now, and I'll work with @tjotala to get it sorted when he's back. |
ewollesen
force-pushed
the
eric-sites
branch
2 times, most recently
from
252debc to
2548f50
Compare
|
@lostlevels I had to rebase onto the latest master, which included the redocly changes, so unfortunately it's kinda a lot of little changes. No logical/functional changes were intended. Just a lot of renaming to match and YAML formatting changes. |
Update: re-based on top of the redocly changes.
Update: re-based on top of changes to remove ordering facility
matching
BACK-3634
After discussion with the team, we will: - no longer have a GET endpoint to list clinic sites - sites creation/update/deletion will no longer return a list of sites - site creation/update will return only the affected site BACK-3632
After discussion with the team, we will: - no longer have a GET endpoint to list patient tags - patient tags creation/update/deletion will no longer return a list of tags - patient tags creation/update will return only the affected tag BACK-3632
The requirement for having patient counts in the returned sites was removed, and since the implementation was clunky, removing it for now seems optimal. BACK-3632
darinkrauss
left a comment
darinkrauss
left a comment
There was a problem hiding this comment.
Some comments. My primary question is should we use siteId model throughout and create siteName model to also use throughout?
You know, I think I had it that way one in one iteration or another, and siteId didn't really yield any benefits, it just sort of hides the fact that it's really just an object id. That said, I'm not opposed to your suggestions, so I'll start working on them. I do have some concerns though, that I would value feedback on. Specifically around the need for QA (re-)review. Given that QA is massively overburdened, and what's here has been approved by QA, I'm reluctant to make changes, IF those changes would require re-review by QA. The changes you've asked for don't seem to me like that would be required. They feel like metadata/organization/nomenclature, i.e. non-functional things. That said, there would be a necessary change to the clinic PR (https://github.com/tidepool-org/clinics/pulls/190) to pick up the latest generated code. What's your opinion on the necessity of a QA re-check on this? UPDATE: After making the requested changes, I feel confident they don't require QA re-review, both because of manually reviewing that they're non-functional, but perhaps more importantly, after I re-generated the bits of clinic affected, 0 changes were required to the non-generated code, which to me very solidly indicates that there are no functional differences. |
In trying to re-use the model of the siteid, which is marked read-only, it became known that marking a path parameter as read-only causes the generated code to ignore the values passed, resulting in 400 bad request errors. So I've duplicated the model-based objectid schema to a property and dropped the read-only property, so now all works as expected. BACK-3632
BACK-3634
REMINDER the backend change to return a 204 on deletion of a site requires a matching frontend change. Deploying to prod without the matching frontend change will break the frontend!